Add type hints to all remaining top-level files #1150

karalekas · 2020-01-02T21:05:32Z

Description

Some typing odds and ends.

Ignore conftest.py, pyquil/magic.py, and all the files in pyquil/_parser
Add typing to all remaining top-level files other than pyquil/paulis.py
Finish off @rht's work and complete the typing for pyquil/paulis.py
Address all feedback (specifically the division issue in pyQVM)

Next PR will be to deal with the API module, which is the only thing left.

Checklist

The above description motivates these changes.
All new and existing tests pass locally and on Travis CI.
Parameters and return values have type hints with PEP 484 syntax.
All code follows Black style and obeys flake8 conventions.
The changelog is updated, including author and PR number (@username, gh-xxx).

notmgsk · 2020-01-02T21:18:43Z

pyquil/wavefunction.py

@@ -147,15 +147,14 @@ def pretty_print_probabilities(self, decimal_digits=2):
                outcome_dict[outcome] = prob
        return outcome_dict

-    def pretty_print(self, decimal_digits=2):
+    def pretty_print(self, decimal_digits: int = 2) -> str:


wicked smart type inference

stylewarning · 2020-01-02T21:47:59Z

but who type checks the types?

stylewarning · 2020-01-02T21:48:44Z

but who type checks the types?

sorry that was a bad "who watches the watchmen" joke

pyquil/operator_estimation.py

pyquil/pyqvm.py

appleby · 2020-01-02T21:51:05Z

pyquil/wavefunction.py

        return len(self.amplitudes).bit_length() - 1

-    def __iter__(self):
+    def __iter__(self) -> Any:


Iterator[complex]?

Oh, this is because numpy is a big ball of untyped anarchy, isn't it?

could we instead cast the returned value to Iterator[complex]?

tried it: mypy doesn't complain, but we also don't actually check that all values in amplitude_vector are complex in __init__, but that is the documented type in the docstring. otoh, Wavefunction.zeros appears to create an amplitude_vector of floats so maybe complex is too restrictive...

pyquil/wavefunction.py

appleby · 2020-01-02T22:02:03Z

but who type checks the types?

mymypy? yourpy?

rht · 2020-01-03T14:29:20Z

but who type checks the types?

mymypy? yourpy?

In fact, mypy is type-checked by itself, e.g. https://github.com/python/mypy/blob/master/mypy/checker.py.

braised-babbage · 2020-01-06T19:01:27Z

pyquil/operator_estimation.py

@@ -305,7 +306,7 @@ def measure_observables(
        # either the first column, second column, or both and multiplying along the row.
        for setting in settings:
            # Get the term's coefficient so we can multiply it in later.
-            coeff = complex(setting.out_operator.coefficient)
+            coeff = complex(cast(complex, setting.out_operator.coefficient))


It's not obvious to me -- why is this needed?

pyquil/pyqvm.py

appleby

lgtm. all comments can be of # type: ignore

appleby · 2020-01-07T14:55:53Z

pyquil/paulis.py

+            pow = result * self
+            assert isinstance(pow, PauliTerm)
+            result = pow


mypy is weird.

normally I prefer isinstance checks, but maybe a cast would be better inside this loop?

yeah this one threw me for a l00p (pun intended). I'll try the cast idea

appleby · 2020-01-07T14:56:55Z

pyquil/paulis.py

@@ -330,24 +340,23 @@ def __add__(self, other: Union[PauliDesignator, ExpressionDesignator]) -> "Pauli
        else:  # is a Number
            return self + PauliTerm("I", 0, other)

-    def __radd__(self, other: ExpressionDesignator) -> "PauliTerm":
+    def __radd__(self, other: ExpressionDesignator) -> "PauliSum":
        """Adds this PauliTerm with a Number.

        :param other: A Number
        :returns: A new PauliTerm


appleby · 2020-01-07T14:59:56Z

pyquil/paulis.py

        return PauliTerm("I", 0, other) + self

-    def __sub__(self, other: Union["PauliTerm", Number]) -> "PauliSum":
+    def __sub__(self, other: Union["PauliTerm", ExpressionDesignator]) -> "PauliSum":
        """Subtracts a PauliTerm from this one.

        :param other: A PauliTerm object or a Number


or an Expression ?

appleby · 2020-01-07T15:13:58Z

pyquil/paulis.py

@@ -899,6 +917,8 @@ def exponential_map(term: PauliTerm) -> Callable[[float], Program]:
    :param term: A pauli term to exponentiate
    :returns: A function that takes an angle parameter and returns a program.
    """
+    assert isinstance(term.coefficient, (float, complex))


Is a float possible here? The type annotation on PauliTerm.coefficient above says it's Union[Expression, complex] and the __init__ normalizes any Number to complex.

I thought so too, but it happens in the unit tests. Unclear to me how, but I think that's a problem for another time

appleby · 2020-01-07T15:15:28Z

pyquil/paulis.py

@@ -964,14 +984,13 @@ def reverse_hack(p: Program) -> Program:
    highest_target_index = None

    for index, op in pauli_term:
+        assert isinstance(index, int) or isinstance(index, QubitPlaceholder)


Suggested change

assert isinstance(index, int) or isinstance(index, QubitPlaceholder)

assert isinstance(index, (int, QubitPlaceholder))

appleby · 2020-01-07T15:16:36Z

pyquil/paulis.py

@@ -984,6 +1003,7 @@ def reverse_hack(p: Program) -> Program:
    # building rotation circuit
    quil_prog += change_to_z_basis
    quil_prog += cnot_seq
+    assert isinstance(pauli_term.coefficient, (float, complex)) and highest_target_index is not None


ditto here re: float

appleby · 2020-01-07T15:32:10Z

pyquil/wavefunction.py

        return len(self.amplitudes).bit_length() - 1

-    def __iter__(self):
+    def __iter__(self) -> Any:


could we instead cast the returned value to Iterator[complex]?

appleby · 2020-01-07T15:32:42Z

pyquil/wavefunction.py

        return self.amplitudes.__iter__()

-    def __getitem__(self, index):
+    def __getitem__(self, index: int) -> Any:


ditto here regarding cast

ill give it a shot

rht · 2020-01-07T16:08:29Z

pyquil/paulis.py

-        self, op: str, index: PauliTargetDesignator, coefficient: ExpressionDesignator = 1.0
+        self,
+        op: str,
+        index: Optional[PauliTargetDesignator],


Why is there an extra Optional[] here?

because for sI() the qubit argument is optional, so this can actually be None

Shouldn't it be index: Optional[PauliTargetDesignator] = None then?

Not necessarily. Despite the name, it's valid (but rare) to have an Optional required argument. Adding the = None makes it a keyword arg, which means it gets a default value if the caller doesn't explicitly provide it. Whereas Optional without = None means you must explicitly pass None when you want index to be None.

Making index a keyword arg here should be backwards compatible, but since it was a required arg before adding the type hints, it seems reasonable for it to remain one after.

karalekas added devops 🚀 An issue related to CI/CD. quality 🎨 Improve code quality. labels Jan 2, 2020

karalekas added this to the v2.16 milestone Jan 2, 2020

karalekas requested a review from a team as a code owner January 2, 2020 21:05

karalekas requested a review from a team January 2, 2020 21:09

notmgsk reviewed Jan 2, 2020

View reviewed changes

notmgsk approved these changes Jan 2, 2020

View reviewed changes

stylewarning approved these changes Jan 2, 2020

View reviewed changes

appleby reviewed Jan 2, 2020

View reviewed changes

karalekas changed the base branch from device-dataclasses to master January 2, 2020 23:34

karalekas force-pushed the pyquil-top-level-typing branch from 1b555c3 to ef81266 Compare January 2, 2020 23:34

braised-babbage reviewed Jan 6, 2020

View reviewed changes

karalekas added 4 commits January 6, 2020 20:09

Add type annotations to nearly all of the top-level files

52308be

Catch remaining typing issues in operator estimation

4e88395

Ignore typing errors in all parser-related files

425f2d7

Update the changelog accordingly

e246fc9

karalekas force-pushed the pyquil-top-level-typing branch from ef81266 to e246fc9 Compare January 7, 2020 04:23

karalekas changed the title ~~Add type annotations to nearly all of the top-level files~~ Add type hints to all remaining top-level files Jan 7, 2020

karalekas added 2 commits January 6, 2020 21:18

Finish off paulis.py

4a49ccc

Address review comments

cb79b75

karalekas requested a review from appleby January 7, 2020 05:45

appleby reviewed Jan 7, 2020

View reviewed changes

rht reviewed Jan 7, 2020

View reviewed changes

Final quomments

bb58bec

karalekas merged commit 70659d1 into master Jan 8, 2020

karalekas deleted the pyquil-top-level-typing branch January 8, 2020 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type hints to all remaining top-level files #1150

Add type hints to all remaining top-level files #1150

karalekas commented Jan 2, 2020 •

edited

Loading

notmgsk Jan 2, 2020

stylewarning commented Jan 2, 2020

stylewarning commented Jan 2, 2020

appleby Jan 2, 2020

appleby Jan 2, 2020

appleby Jan 7, 2020

appleby Jan 7, 2020

appleby commented Jan 2, 2020

rht commented Jan 3, 2020

braised-babbage Jan 6, 2020

appleby left a comment

appleby Jan 7, 2020

karalekas Jan 8, 2020

appleby Jan 7, 2020

appleby Jan 7, 2020

appleby Jan 7, 2020

karalekas Jan 8, 2020

appleby Jan 7, 2020

appleby Jan 7, 2020

karalekas Jan 8, 2020

appleby Jan 7, 2020

appleby Jan 7, 2020

karalekas Jan 8, 2020

rht Jan 7, 2020

karalekas Jan 8, 2020

rht Jan 8, 2020

appleby Jan 8, 2020

	assert isinstance(index, int) or isinstance(index, QubitPlaceholder)
	assert isinstance(index, (int, QubitPlaceholder))

Add type hints to all remaining top-level files #1150

Add type hints to all remaining top-level files #1150

Conversation

karalekas commented Jan 2, 2020 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

stylewarning commented Jan 2, 2020

stylewarning commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appleby commented Jan 2, 2020

rht commented Jan 3, 2020

Choose a reason for hiding this comment

appleby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalekas commented Jan 2, 2020 •

edited

Loading